Fixed dangling else: in issuer_or_subject_length() #35
Conversation
|
Fixes issue #30 . |
|
@DJDevon3 Since you looked at this library recently, would you be able to review this PR? |
|
@42xnor I don't think checking for 0 is the right approach for a CSR. In my opinion all fields (except organization unit) should be required. Each field adds 11 plus len(data). By checking for 0 you could fill in 1 field with 1 digit and it would pass the raise but still fail the rest as empty str with a type error. To make a field optional it should have = None added to the type. I think it's a better idea to annotate that most fields are required. This is in better spirit of a CSR. You don't have to fill it in with factual information but you do at least have to fill it with something. def issuer_or_subject_length(country: str, state_prov: str, city: str, org: str, common: str, org_unit: str = None) -> int:
"""
Returns total length of provided certificate information.
:param str country: Country of certificate (Required)
:param str state_prov: State/province of certificate (Required)
:param str city: City of certificate (Required)
:param str org: Organization of certificate (Required)
:param str common: Common data of certificate (Required)
:param str org_unit: Organization unit of certificate (Optional)
:raises: TypeError if return value is less than 55 or if any required field is empty
:return: Total length of provided certificate information.
"""
empty_fields = [field_name for field_name, field_value in {"Country": country, "State/Province": state_prov, "City": city, "Organization": org, "Common Data": common}.items() if not field_value]
if empty_fields or (org_unit == ''):
raise TypeError(f"The following Required fields cannot be empty: {', '.join(empty_fields)}.")
info_lengths = [11 + len(item) for item in (country, state_prov, city, org, common) if item]
total_length = sum(info_lengths)
if total_length < 55:
raise TypeError("Required fields cannot be blank.")
return total_lengthSince each required field is prepended with 11 and there are 5 required fields then 55 should be the minimum to check against. I've also added some code that will specify which required fields were left blank during the request so they can be filled in. Here's an example to try with the function. import busio
import board
from adafruit_atecc.adafruit_atecc import ATECC
import adafruit_atecc.adafruit_atecc_asn1 as asn1
i2c = busio.I2C(board.SCL, board.SDA, frequency=70000)
atecc = ATECC(i2c, address=0x60, debug=False)
# two fields left blank intentionally to trigger an error
issuer = asn1.issuer_or_subject_length(country="", state_prov="", city="New York", org="Adafruit", common=atecc.serial_number, org_unit=None)
print(f"Issuer Fields Length: {issuer}")Serial output: code.py output:
Traceback (most recent call last):
File "code.py", line 9, in <module>
File "/lib/adafruit_atecc/adafruit_atecc_asn1.py", line 259, in issuer_or_subject_length
TypeError: The following Required fields cannot be empty: State/Province, Country.Is this acceptable or did I completely miss the boat? |
|
and when completely filled in and no optional org unit issuer = asn1.issuer_or_subject_length(country="US", state_prov="NY", city="New York", org="Adafruit", common=atecc.serial_number)
print(f"Issuer Fields Length: {issuer}")serial output as long as it's greater than 55 it'll pass. |
|
After thinking about it, the change I proposed would break any pre-existing chips that have any empty fields in the now required fields. I'm conflicted about this and will have to bring this up in a circuit python meeting. Also the parameter |
Changed the
elseto anif tot_len == 0:to match the doctring's listed functionality "raises: TypeError if return value is 0". The previous code would raiseTypeErrorif the parametercommonwas falsy. It now raises the error iftot_lenis zero.